plugins/filter_lookup/lookup.h (1)

28-31: Use an enum for metric IDs and confirm legacy metrics exposure

Minor: prefer an enum to group these IDs and avoid macro leakage. Also, since the runtime tests fetch metrics via flb_metrics_get_id(), please confirm the plugin populates the legacy metrics list (f_ins->metrics) in addition to cmetrics; otherwise tests on builds without FLB_HAVE_METRICS will fail.

Apply:

-#define FLB_LOOKUP_METRIC_PROCESSED     200
-#define FLB_LOOKUP_METRIC_MATCHED       201
-#define FLB_LOOKUP_METRIC_SKIPPED       202
+enum {
+    FLB_LOOKUP_METRIC_PROCESSED = 200,
+    FLB_LOOKUP_METRIC_MATCHED   = 201,
+    FLB_LOOKUP_METRIC_SKIPPED   = 202
+};

tests/runtime/filter_lookup.c (2)

125-145: Don’t assume null-terminated output; respect size

The lib output callback receives a buffer and length. Using strstr() on an assumed NUL-terminated buffer is brittle. Copy to a NUL-terminated scratch buffer first.

 static int cb_check_result_json(void *record, size_t size, void *data)
 {
-    char *p;
-    char *expected;
-    char *result;
+    char *p;
+    char *expected;
+    char *result;
 
     expected = (char *) data;
-    result = (char *) record;
+    result = flb_malloc(size + 1);
+    if (!result) {
+        flb_free(record);
+        return -1;
+    }
+    memcpy(result, record, size);
+    result[size] = '\0';
 
     p = strstr(result, expected);
     TEST_CHECK(p != NULL);
 
     if (p == NULL) {
         flb_error("Expected to find: '%s' in result '%s'",
                   expected, result);
     }
 
-    flb_free(record);
+    flb_free(result);
+    flb_free(record);
     return 0;
 }

---

449-497: Strengthen “no match” test: assert result_key is absent

Currently this test only checks that another field exists. Add a negative assertion to ensure the filter didn’t inject result_key on misses.

+/* Callback to assert a substring is NOT present */
+static int cb_assert_not_contains(void *record, size_t size, void *data)
+{
+    char *needle = (char *) data;
+    char *buf = flb_malloc(size + 1);
+    if (!buf) { flb_free(record); return -1; }
+    memcpy(buf, record, size);
+    buf[size] = '\0';
+    TEST_CHECK(strstr(buf, needle) == NULL);
+    if (strstr(buf, needle) != NULL) {
+        flb_error("Unexpected substring '%s' found in: '%s'", needle, buf);
+    }
+    flb_free(buf);
+    flb_free(record);
+    return 0;
+}
@@
-    /* Should NOT contain the result_key since no match was found */
-    cb_data.cb = cb_check_result_json;
-    cb_data.data = "\"other_field\":\"test\"";
+    /* Should NOT contain result_key since no match was found */
+    cb_data.cb = cb_assert_not_contains;
+    cb_data.data = "\"user_name\":";

plugins/filter_lookup/lookup.c (7)

766-768: Float formatting may not match CSV keys; use compact representation.

%f prints 6 decimals (e.g., 1.0 → 1.000000). Prefer %.15g to preserve intent and improve match rates.

-                case FLB_RA_FLOAT:
-                    required_size = snprintf(NULL, 0, "%f", rval->o.via.f64);
+                case FLB_RA_FLOAT:
+                    required_size = snprintf(NULL, 0, "%.15g", rval->o.via.f64);
                     break;
...
-                case FLB_RA_FLOAT:
-                    printed = snprintf(dynamic_val_buf, required_size + 1, "%f", rval->o.via.f64);
+                case FLB_RA_FLOAT:
+                    printed = snprintf(dynamic_val_buf, required_size + 1, "%.15g", rval->o.via.f64);
                     break;

Also applies to: 815-817

---

46-74: Metrics macros: avoid static mutable storage and guard counter pointers.

static char* labels_array[1]; is shared across threads. Also, cmt_counter_create() can fail; guard ctx->cmt_* in the macros.

-#define INCREMENT_SKIPPED_METRIC(ctx, ins) do { \
-    uint64_t ts = cfl_time_now(); \
-    static char* labels_array[1]; \
-    labels_array[0] = (char*)flb_filter_name(ins); \
-    cmt_counter_add(ctx->cmt_skipped, ts, 1, 1, labels_array); \
-    flb_metrics_sum(FLB_LOOKUP_METRIC_SKIPPED, 1, ins->metrics); \
-} while(0)
+#define INCREMENT_SKIPPED_METRIC(ctx, ins) do { \
+    uint64_t ts = cfl_time_now(); \
+    char* labels_array[1]; \
+    labels_array[0] = (char *) flb_filter_name(ins); \
+    if ((ctx)->cmt_skipped) { cmt_counter_add((ctx)->cmt_skipped, ts, 1, 1, labels_array); } \
+    if ((ins)->metrics) { flb_metrics_sum(FLB_LOOKUP_METRIC_SKIPPED, 1, (ins)->metrics); } \
+} while (0)

Apply the same pattern to INCREMENT_MATCHED_METRIC and INCREMENT_PROCESSED_METRIC.

---

529-547: Defensive checks on metric creation.

If any cmt_counter_create() returns NULL, later increments will segfault without guards. Consider logging and continuing without those counters.

-        ctx->cmt_processed = cmt_counter_create(ins->cmt,
+        ctx->cmt_processed = cmt_counter_create(ins->cmt,
                                                 "fluentbit", "filter", "lookup_processed_records_total",
                                                 "Total number of processed records",
                                                 1, labels_name);
+        if (!ctx->cmt_processed) { flb_plg_warn(ins, "failed to create processed counter"); }
...
-        ctx->cmt_matched = cmt_counter_create(ins->cmt,
+        ctx->cmt_matched = cmt_counter_create(ins->cmt,
                                               "fluentbit", "filter", "lookup_matched_records_total",
                                               "Total number of matched records",
                                               1, labels_name);
+        if (!ctx->cmt_matched) { flb_plg_warn(ins, "failed to create matched counter"); }
...
-        ctx->cmt_skipped = cmt_counter_create(ins->cmt,
+        ctx->cmt_skipped = cmt_counter_create(ins->cmt,
                                               "fluentbit", "filter", "lookup_skipped_records_total",
                                               "Total number of skipped records due to errors",
                                               1, labels_name);
+        if (!ctx->cmt_skipped) { flb_plg_warn(ins, "failed to create skipped counter"); }

---

261-269: Always skipping the first CSV line assumes a header. Make this configurable.

Not all CSVs include a header; the first record would be dropped. Add a skip_header (bool) option or auto-detect, and document the behavior.

I can draft the config plumb-through and tests if you want it in this PR.

---

606-609: Avoid relying on internal ht->total_count.

Accessing struct internals risks future ABI churn. Track a local “loaded entries” count in load_csv() and log that instead.

---

919-938: Precompute result_key length and use memcmp.

Minor perf/readability tweak in the hot path.

-        if (log_event.body && log_event.body->type == MSGPACK_OBJECT_MAP) {
+        if (log_event.body && log_event.body->type == MSGPACK_OBJECT_MAP) {
             int i;
+            size_t rkey_len = strlen(ctx->result_key);
             for (i = 0; i < log_event.body->via.map.size; i++) {
                 msgpack_object_kv *kv = &log_event.body->via.map.ptr[i];
                 if (kv->key.type == MSGPACK_OBJECT_STR &&
-                    kv->key.via.str.size == strlen(ctx->result_key) &&
-                    strncmp(kv->key.via.str.ptr, ctx->result_key, kv->key.via.str.size) == 0) {
+                    kv->key.via.str.size == rkey_len &&
+                    memcmp(kv->key.via.str.ptr, ctx->result_key, rkey_len) == 0) {
                     continue;
                 }

---

183-244: CSV support limitations (multiline fields).

Reader splits on newline before CSV parsing; embedded newlines within quoted fields aren’t supported. If out-of-scope, document explicitly.

Also applies to: 291-417

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro